Feat: Implement support for Guest Users #4514
Conversation
📝 WalkthroughWalkthroughThe changes introduce a comprehensive guest user management feature into the auth service. A new controller handles guest user operations (create, read, update, convert, list, delete) and leverages utility functions for processing and error management. A corresponding Mongoose model defines the guest user schema and registration logic. Additionally, new Express routes, filter generation logic, and validation middleware are implemented to support and secure these operations. The new routes are integrated into the API, ensuring consistent processing of guest user requests through proper error handling and response formatting. Changes
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #4514 +/- ##
===========================================
- Coverage 11.57% 11.51% -0.07%
===========================================
Files 159 155 -4
Lines 18733 18709 -24
Branches 547 549 +2
===========================================
- Hits 2169 2154 -15
+ Misses 16560 16551 -9
Partials 4 4
|
Baalmart
changed the title
|
Auth-service changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/auth-service/models/GuestUser.js (2)
36-51: Consider different response structure for failed creation.The response when a guest user is not successfully created still has
success: true, which could be misleading. Consider settingsuccess: falsefor clarity.} else { return { - success: true, + success: false, data: createdGuestUser, message: "Operation successful but guest user NOT successfully created", status: httpStatus.OK, }; }
64-74: Model retrieval logic can be simplified.The current implementation tries to retrieve the model first and creates it if there's an error. A more direct approach would be to check if the model exists first using
mongoose.modelNames().const GuestUserModel = (tenant) => { const defaultTenant = constants.DEFAULT_TENANT || "airqo"; const dbTenant = isEmpty(tenant) ? defaultTenant : tenant; try { - let users = mongoose.model("guest_users"); - return users; - } catch (error) { - let users = getModelByTenant(dbTenant, "guest_user", GuestUserSchema); - return users; + const modelName = "guest_users"; + return mongoose.modelNames().includes(modelName) + ? mongoose.model(modelName) + : getModelByTenant(dbTenant, "guest_user", GuestUserSchema); + } catch (error) { + logger.error(`Error retrieving guest user model: ${error.message}`); + throw error; } };src/auth-service/routes/v2/guests.routes.js (1)
7-16: Consider using Express middleware for CORS.Instead of implementing a custom CORS headers middleware, consider using the widely-adopted
corspackage which provides more comprehensive CORS handling and is better maintained.const express = require("express"); const router = express.Router(); +const cors = require("cors"); const guestUserController = require("@controllers/guest-user.controller"); const guestUserValidations = require("@validators/guest-user.validators"); const { setJWTAuth, authJWT } = require("@middleware/passport"); -const headers = (req, res, next) => { - res.header("Access-Control-Allow-Origin", "*"); - res.header( - "Access-Control-Allow-Headers", - "Origin, X-Requested-With, Content-Type, Accept, Authorization" - ); - res.header("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, PATCH"); - next(); -}; -router.use(headers); +router.use(cors({ + origin: "*", + methods: ["GET", "POST", "PUT", "DELETE", "PATCH"], + allowedHeaders: ["Origin", "X-Requested-With", "Content-Type", "Accept", "Authorization"] +}));src/auth-service/utils/guest-user.util.js (4)
11-31: Potential duplication check for new guest usersCurrently, the
createfunction registers a new guest user without verifying if a user with the same email or data already exists. This could lead to duplicate guest accounts. Consider adding a check or letting theregistermethod handle duplicates gracefully.
32-93: Incomplete data migration logicThe comment “migrate data from guest to new user” indicates there’s more data that should be transferred during guest conversion. If additional fields need to be moved, consider finalizing this logic to prevent data loss or inconsistencies.
Do you want assistance implementing the data migration logic?
141-162: Guest deletion appears straightforwardConsider auditing any related records or data (e.g., logs, references in other tables/collections) if a guest user is removed.
163-196: Return 404 instead of 200 when not foundCurrently, the code returns a success status even if the guest user doesn’t exist. To align with REST best practices, consider returning an HTTP 404 status for not found results.
src/auth-service/validators/guest-user.validators.js (2)
19-37: Validate tenant valuesHardcoding tenant values is fine for a small set, but if additional tenants are expected, consider dynamically loading from configuration. Also, verify that "airqount" is the intended string.
64-84: Additional validations for email or passwordConsider adding stricter rules for email format (
.isEmail()) and password complexity (e.g.,.isLength({ min: 8 })).src/auth-service/controllers/guest-user.controller.js (2)
17-67: Repeated pre-processing logicChecking for request errors, setting a default tenant, and calling the utility is repeated in every method. Consider extracting this logic into a shared helper to reduce duplication.
168-217: Pagination clarityWhen listing guest users, consider including pagination info (like totalCount, skip, limit) in the response to make it more user-friendly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/auth-service/controllers/guest-user.controller.js(1 hunks)src/auth-service/models/GuestUser.js(1 hunks)src/auth-service/routes/v2/guests.routes.js(1 hunks)src/auth-service/routes/v2/index.js(1 hunks)src/auth-service/utils/common/generate-filter.js(1 hunks)src/auth-service/utils/guest-user.util.js(1 hunks)src/auth-service/validators/guest-user.validators.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
src/auth-service/routes/v2/index.js (1)
30-30: Integration of guest users route looks good.The new route for guest users is properly integrated into the existing router structure. The placement before the catch-all users route ("/") is appropriate, as more specific routes should come before general ones.
src/auth-service/utils/common/generate-filter.js (1)
99-127: Well-structured filter implementation for guest users.The implementation follows the established pattern in the codebase for filter generation. The conversion of tenant to lowercase ensures consistent filtering regardless of input case, and the proper handling of MongoDB ObjectId for the id field is correct.
src/auth-service/models/GuestUser.js (1)
12-23: Schema design is appropriate for guest user requirements.The schema includes essential fields with proper indexing. Using
timestamps: trueis a good practice for tracking creation and update times automatically.src/auth-service/routes/v2/guests.routes.js (3)
21-27: List endpoint properly secured with authentication.The guest user listing endpoint is appropriately secured with JWT authentication middleware, ensuring that only authenticated users can access this potentially sensitive data.
28-48: Consistent authentication pattern for modification endpoints.The update, delete, and getOne endpoints all follow a consistent pattern of validation followed by authentication, which is good practice for security and maintainability.
19-20:❓ Verification inconclusive
Creating guest users doesn't require authentication.
The POST route for creating guest users doesn't require authentication, which is appropriate for enabling unauthenticated users to start using the system. However, make sure this aligns with your security requirements.
🏁 Script executed:
#!/bin/bash # Check if there are any security validations in the guest user creation controller grep -n "require\(\|verif\|check\|guard\|secure\|valid" src/auth-service/controllers/guest-user.controller.js | grep -i "create\|guest"Length of output: 161
I'll re-run the search for security validations using a corrected regex so we can verify whether any checks exist in the guest user creation controller. Please run the following script:
Based on the intended design, creating guest users is supposed to be unauthenticated. If this updated check returns no output, it confirms that there are no hidden security validations—for now—which aligns with the design. Still, please verify manually that this behavior meets your overall security requirements.
Guest User Creation Endpoint – Unauthenticated by Design
The guest user creation route (using
router.post("/", guestUserValidations.create, guestUserController.create)) is deliberately left open to allow unauthenticated users to start using the system. No extra security validations were found in the controller for guest creation, so this behavior appears intentional. Please ensure that this design decision fits your security requirements, knowing that the conversion endpoint remains protected.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run the check for any security validations in the guest-user.controller.js file grep -En "require\(|verif|check|guard|secure|valid" src/auth-service/controllers/guest-user.controller.js | grep -Ei "create|guest"Length of output: 314
Guest User Creation Endpoint – Unauthenticated by Design (Re-verified)
The updated check in the guest user controller only revealed the import of the guest user utility:
const guestUserUtil = require("@utils/guest-user.util");No additional security validations or authentication checks were detected. This confirms that the unauthenticated guest user creation route defined in:
router.post("/", guestUserValidations.create, guestUserController.create); router.post("/convert", setJWTAuth, authJWT, guestUserController.convertGuest);is intentional for allowing unauthenticated users to initiate use of the system. Please ensure that this design decision is in line with your overall security requirements, keeping in mind that the conversion endpoint remains protected.
src/auth-service/utils/guest-user.util.js (3)
1-9: Imports look consistentThe chosen modules appear relevant and well-organized.
94-116: Filtering approach is solidThe function effectively uses
generateFilter.guest_users(), skip, and limit. This is a standard approach for paginated listing.
117-140: Update logicThe function merges
bodyinto an update object but doesn't apply any transformations or validations beyond what's in the validators. Ensure that fields likeguest_idor other unique fields are not changed inadvertently if that violates your data model.src/auth-service/validators/guest-user.validators.js (2)
5-17: Pagination defaults look goodThe logic ensures valid numeric values for
limitandskip, with sensible defaults and upper bounds.
39-54: Robust ID parameter validationEnsuring it’s a valid MongoDB ObjectId is helpful. This helps avoid cast errors in queries.
src/auth-service/controllers/guest-user.controller.js (5)
1-16: Structured importsThe controller’s imports are well-organized and relevant.
68-117: Consistent approach for retrievalThe logic for
getOneis straightforward and consistent with the rest of the controller.
118-167: Verify error coverageEnsure that any potential errors from the underlying
convertGuestutility are appropriately handled. For instance, if data migration fails or if the user already exists, confirm that the utility setsresult.successto false so this block can handle it gracefully.
219-268: Update method logicImplementation is consistent with other endpoints, and error handling is robust.
269-319: Deletion flow is consistentError handling and success response structure is consistent with the overall pattern.
src/auth-service/models/GuestUser.js
Outdated
| async register(args, next) { | ||
| try { | ||
| const guestId = accessCodeGenerator | ||
| .generate(constants.RANDOM_PASSWORD_CONFIGURATION(16)) | ||
| .toUpperCase(); | ||
|
|
||
| const createdGuestUser = await this.create({ | ||
| guest_id: guestId, | ||
| ...args, | ||
| }); | ||
|
|
||
| if (!isEmpty(createdGuestUser)) { | ||
| return { | ||
| success: true, | ||
| data: createdGuestUser, | ||
| message: "guest user created", | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else { | ||
| return { | ||
| success: true, | ||
| data: createdGuestUser, | ||
| message: | ||
| "Operation successful but guest user NOT successfully created", | ||
| status: httpStatus.OK, | ||
| }; | ||
| } | ||
| } catch (err) { | ||
| logger.error(`🐛🐛 Internal Server Error -- ${err.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: err.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Missing import for isEmpty function in register method.
The register method uses isEmpty() at line 36, but there's no import for this function in the file.
Add the following import at the top of the file:
const httpStatus = require("http-status");
+const isEmpty = require("is-empty");
const { logObject, logText, HttpError } = require("@utils/shared");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async register(args, next) { | |
| try { | |
| const guestId = accessCodeGenerator | |
| .generate(constants.RANDOM_PASSWORD_CONFIGURATION(16)) | |
| .toUpperCase(); | |
| const createdGuestUser = await this.create({ | |
| guest_id: guestId, | |
| ...args, | |
| }); | |
| if (!isEmpty(createdGuestUser)) { | |
| return { | |
| success: true, | |
| data: createdGuestUser, | |
| message: "guest user created", | |
| status: httpStatus.OK, | |
| }; | |
| } else { | |
| return { | |
| success: true, | |
| data: createdGuestUser, | |
| message: | |
| "Operation successful but guest user NOT successfully created", | |
| status: httpStatus.OK, | |
| }; | |
| } | |
| } catch (err) { | |
| logger.error(`🐛🐛 Internal Server Error -- ${err.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: err.message } | |
| ) | |
| ); | |
| } | |
| }, | |
| }; | |
| // File: src/auth-service/models/GuestUser.js | |
| const httpStatus = require("http-status"); | |
| const isEmpty = require("is-empty"); | |
| const { logObject, logText, HttpError } = require("@utils/shared"); | |
| // ... other imports and code ... | |
| async register(args, next) { | |
| try { | |
| const guestId = accessCodeGenerator | |
| .generate(constants.RANDOM_PASSWORD_CONFIGURATION(16)) | |
| .toUpperCase(); | |
| const createdGuestUser = await this.create({ | |
| guest_id: guestId, | |
| ...args, | |
| }); | |
| if (!isEmpty(createdGuestUser)) { | |
| return { | |
| success: true, | |
| data: createdGuestUser, | |
| message: "guest user created", | |
| status: httpStatus.OK, | |
| }; | |
| } else { | |
| return { | |
| success: true, | |
| data: createdGuestUser, | |
| message: | |
| "Operation successful but guest user NOT successfully created", | |
| status: httpStatus.OK, | |
| }; | |
| } | |
| } catch (err) { | |
| logger.error(`🐛🐛 Internal Server Error -- ${err.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: err.message } | |
| ) | |
| ); | |
| } | |
| }, |
|
Auth-service changes in this PR available for preview here |
|
Auth-service changes in this PR available for preview here |
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
src/auth-service/utils/common/generate-filter.js (1)
101-105: Consider extracting additional filter parameters.Most other filter methods in this file extract multiple parameters from requests, but this implementation only uses
idandguest_id. Depending on the GuestUser schema and query requirements, you might need additional filters liketenant, which appears in other filter methods.- let { id, guest_id } = { + let { id, guest_id, tenant, lastActive } = { ...req.body, ...req.query, ...req.params, };src/auth-service/validators/common/validate.validators.js (1)
7-10: Consider adding request identifiers for better error traceability.For enhanced monitoring and debugging capabilities, consider adding a unique request identifier in the error object. This would help trace validation failures across logs and systems.
return next( - new HttpError("Validation Error", httpStatus.BAD_REQUEST, errors) + new HttpError("Validation Error", httpStatus.BAD_REQUEST, errors, req.id || undefined) ); // next with HttpErrorYou would need to modify your HttpError class to accept and store this identifier if it doesn't already.
src/auth-service/validators/common/headers.validators.js (3)
1-5: Commented code should be removed or properly documented.There's a substantial amount of commented code at the beginning of this file that appears to be an alternate implementation using configurable allowed origins. Either remove this code entirely or add a comment explaining why it's preserved for future reference.
-// const constants = require("@config/constants"); -// const allowedOrigins = -// constants.ALLOWED_ORIGINS && constants.ALLOWED_ORIGINS.trim() !== "" -// ? constants.ALLOWED_ORIGINS.split(",").map((origin) => origin.trim()) -// : ["*"]; +/* + * Note: This more restrictive CORS implementation was preserved for future use + * when we need to limit allowed origins based on environment configuration. + * + * const constants = require("@config/constants"); + * const allowedOrigins = + * constants.ALLOWED_ORIGINS && constants.ALLOWED_ORIGINS.trim() !== "" + * ? constants.ALLOWED_ORIGINS.split(",").map((origin) => origin.trim()) + * : ["*"]; + */
8-16: Remove or document these commented sections for clarity.Similar to the previous comment, there's more commented code that should either be removed or properly documented with an explanation for why it's being preserved.
-// const origin = req.get("Origin"); -// res.setHeader( -// "Access-Control-Allow-Origin", -// allowedOrigins.includes("*") -// ? "*" -// : allowedOrigins.includes(origin) -// ? origin -// : "" -// ); +/* + * This is the implementation that would work with the configurable allowed origins. + * It would set the Access-Control-Allow-Origin header based on the requesting origin. + */
17-23: Consider security implications of allowing all origins.Using "*" for Access-Control-Allow-Origin allows requests from any origin, which may present security risks in a production environment. Consider implementing a more restrictive CORS policy that only allows specific, trusted origins.
You could implement a more secure approach by using an environment-specific configuration:
-res.setHeader("Access-Control-Allow-Origin", "*"); +const allowedOrigins = process.env.ALLOWED_ORIGINS + ? process.env.ALLOWED_ORIGINS.split(',').map(origin => origin.trim()) + : []; +const origin = req.get('Origin'); + +if (process.env.NODE_ENV === 'development' || allowedOrigins.includes('*')) { + res.setHeader("Access-Control-Allow-Origin", "*"); +} else if (origin && allowedOrigins.includes(origin)) { + res.setHeader("Access-Control-Allow-Origin", origin); +}This approach would maintain the convenience of "*" in development but enforce a stricter policy in production.
src/auth-service/validators/common/pagination.validators.js (2)
15-16: Clarify the comment about skip assignment.The comment "Assign 0 directly to skip. No need for req.query" is slightly confusing since req.query.skip is still assigned later. Consider clarifying or removing this comment.
if (Number.isNaN(skip) || skip < 0) { - skip = 0; // Assign 0 directly to skip. No need for req.query + skip = 0; // Reset to 0 for invalid values }
8-13: Consider adding a response header for actual limits applied.To improve API transparency, consider adding response headers that indicate the actual limit and maximum limit values applied to the request. This helps clients understand pagination constraints and adapt accordingly.
if (Number.isNaN(limit) || limit < 1) { limit = defaultLimit; } if (limit > maxLimit) { limit = maxLimit; } + +// Set headers to inform client about applied limits +res.setHeader('X-Pagination-Limit', limit); +res.setHeader('X-Pagination-Max-Limit', maxLimit);src/auth-service/routes/v2/guests.routes.js (2)
18-18: Clarify the use of global pagination middleware.The route is using
guestUserValidations.paginationglobally for all routes, but pagination typically only applies to list operations. Consider moving this middleware to only apply to the relevant GET routes.-router.use(guestUserValidations.pagination); +// Only apply pagination middleware to the list endpoint +router.get( + "/", + guestUserValidations.pagination, + guestUserValidations.list, + setJWTAuth, + authJWT, + validate, + guestUserController.list +);This assumes you would then remove the pagination middleware from the existing GET route or adjust its implementation accordingly.
26-32: Add documentation for guest conversion process.The
/convertendpoint is crucial but lacks documentation about what happens during conversion. Consider adding JSDoc or similar documentation to explain the conversion process, required parameters, and expected outcomes.+/** + * @route POST /v2/guests/convert + * @description Converts a guest user to a full user account + * @access Private - Requires authentication + * @body {string} guest_id - ID of the guest user to convert + * @body {object} userData - User data for the new account + * @returns {object} The newly created full user account + */ router.post( "/convert", setJWTAuth, authJWT, validate, guestUserController.convertGuest );src/auth-service/models/GuestUser.js (3)
1-24: Be mindful of logging details in production.Mongoose
debugmode can increase verbosity and potentially reveal sensitive info. Ensure it's disabled or carefully managed in production to prevent exposing sensitive data in logs.
26-63: Clarify handling of 'empty' creation result.
this.create(...)typically returns a valid document or throws an error, making the “empty” check less likely. Consider treating an empty result as an error or refining the logic for better clarity.
224-224: Add unit tests for the static methods.Testing the registration, listing, modification, removal, and counting of guest users ensures reliability over time.
Would you like assistance creating a new test file to provide coverage for these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/auth-service/models/GuestUser.js(1 hunks)src/auth-service/routes/v2/guests.routes.js(1 hunks)src/auth-service/utils/common/generate-filter.js(1 hunks)src/auth-service/validators/common/headers.validators.js(1 hunks)src/auth-service/validators/common/index.js(1 hunks)src/auth-service/validators/common/pagination.validators.js(1 hunks)src/auth-service/validators/common/validate.validators.js(1 hunks)src/auth-service/validators/guest-user.validators.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/auth-service/validators/common/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/auth-service/validators/guest-user.validators.js
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/auth-service/utils/common/generate-filter.js
[warning] 99-101: src/auth-service/utils/common/generate-filter.js#L99-L101
Added lines #L99 - L101 were not covered by tests
[warning] 106-106: src/auth-service/utils/common/generate-filter.js#L106
Added line #L106 was not covered by tests
[warning] 108-108: src/auth-service/utils/common/generate-filter.js#L108
Added line #L108 was not covered by tests
[warning] 111-111: src/auth-service/utils/common/generate-filter.js#L111
Added line #L111 was not covered by tests
[warning] 114-114: src/auth-service/utils/common/generate-filter.js#L114
Added line #L114 was not covered by tests
[warning] 116-117: src/auth-service/utils/common/generate-filter.js#L116-L117
Added lines #L116 - L117 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
src/auth-service/utils/common/generate-filter.js (2)
116-124: Error handling implementation is consistent with other methods.The error handling approach mirrors that of other methods in this file, which is good for consistency. The logging and error forwarding with appropriate HTTP status codes will help with debugging and proper client notifications.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 116-117: src/auth-service/utils/common/generate-filter.js#L116-L117
Added lines #L116 - L117 were not covered by tests
107-112:✅ Verification successful
Keep filter field population consistent with the GuestUser schema.
The implementation appropriately converts
idto ObjectId but keepsguest_idas a string. Ensure this matches the schema definition of the GuestUser model for proper query functionality.
🏁 Script executed:
#!/bin/bash # Find the GuestUser model schema to confirm field types echo "Searching for GuestUser model definition..." rg -A 10 "const GuestUserSchema = new (mongoose\.Schema|Schema)\(" --type js echo "Checking for guest_id field usage in other files..." rg "guest_id" --type jsLength of output: 1898
GuestUser Schema Consistency Verified
The GuestUser schema definesguest_idas a string (as shown insrc/auth-service/models/GuestUser.js), and the filter ingenerate-filter.jsis correctly settingguest_idwithout a conversion. The implementation is consistent with the schema, so no changes are necessary.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-108: src/auth-service/utils/common/generate-filter.js#L108
Added line #L108 was not covered by tests
[warning] 111-111: src/auth-service/utils/common/generate-filter.js#L111
Added line #L111 was not covered by testssrc/auth-service/validators/common/validate.validators.js (1)
1-14: Well-designed validation middleware with clear error handling.This is a well-crafted validation middleware function that follows established patterns. The implementation properly extracts errors from requests, checks their existence, and either passes the errors as an appropriately formatted HTTP error or proceeds to the next middleware in the stack. The structure is clean and the error response includes all necessary components (message, status code, and detailed errors).
src/auth-service/validators/common/pagination.validators.js (1)
1-23: Well-implemented pagination middleware with proper validation.This pagination middleware provides a robust approach to sanitizing and constraining pagination parameters. The implementation includes appropriate type checking, range validation, and sensible defaults. The middleware correctly applies only to GET requests with pagination parameters, ensuring it doesn't interfere with other request types.
src/auth-service/routes/v2/guests.routes.js (2)
41-64: Consistent authentication and validation flow across endpoints.The route definitions for update, delete, and getOne operations follow a consistent pattern of validation, authentication, and controller invocation. This is a good practice for maintaining predictable API behavior and security.
28-32:❓ Verification inconclusive
Ensure proper user identification during guest conversion.
For the
/convertendpoint that converts a guest user to a full user, verify that you're properly identifying which guest account belongs to the authenticated user. This prevents unauthorized conversions of other users' guest accounts.
🏁 Script executed:
#!/bin/bash # Check how guest user ownership is verified during conversion # Search for the implementation of convertGuest to see how it validates ownership echo "Examining guest user conversion implementation:" rg -A 10 "convertGuest" --glob "src/auth-service/controllers/**"Length of output: 2079
Clarification on Guest Account Validation
Before merging, please confirm that the conversion logic within
guestUserUtil.convertGuestrobustly verifies that the authenticated user is indeed the owner of the guest account being converted. The controller insrc/auth-service/controllers/guest-user.controller.jsdelegates the conversion without an explicit verification check in the snippet—verify that the utility method correctly compares the authenticated user context against the guest account to prevent unauthorized conversions. If this check isn’t sufficiently enforced, consider adding explicit validation and corresponding test cases.src/auth-service/models/GuestUser.js (5)
64-92: Listing logic looks solid.This method correctly handles empty results and logs essential information.
103-132: Update logic is straightforward.The function uses
findOneAndUpdatewith{ new: true }to return the updated doc and properly handles the not-found scenario.
133-161: Removal logic is consistent.Using
findOneAndRemoveis appropriate here, and the response states when the document is not found.
162-181: Count logic is properly implemented.The method cleanly returns the total number of matching guest users.
182-210: findOne method is well-structured.It returns a 404 status when not found and a 200 status otherwise, matching standard REST conventions.
| guest_users: (req, next) => { | ||
| try { | ||
| let { id, guest_id } = { | ||
| ...req.body, | ||
| ...req.query, | ||
| ...req.params, | ||
| }; | ||
| let filter = {}; | ||
| if (id) { | ||
| filter["_id"] = ObjectId(id); | ||
| } | ||
| if (guest_id) { | ||
| filter["guest_id"] = guest_id; | ||
| } | ||
|
|
||
| return filter; | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
The guest_users filter implementation looks good, but lacks test coverage.
The implementation follows the established pattern of other filter methods in this file by extracting relevant parameters from the request and constructing a filter object. However, the static analysis indicates that this new code lacks test coverage, which is crucial for authentication-related functionality.
🏁 Script executed:
#!/bin/bash
# Check for test files that might cover this new guest_users filter functionality
echo "Searching for test files related to guest user functionality..."
fd -e js -e ts test guest.*filter
echo "Searching for any imports or usages of the guest_users filter function..."
rg "filter.guest_users" --type jsLength of output: 466
Guest Users Filter: Add Test Coverage
The implementation of the guest_users filter is consistent with our established patterns, but our verification indicates that there are no tests covering this functionality. Given its importance in authentication, please add unit tests to ensure robust behavior and proper error handling.
- File:
src/auth-service/utils/common/generate-filter.js(lines 99–125) - Action: Add appropriate test cases for the
guest_usersfilter to verify both successful filter construction and error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 99-101: src/auth-service/utils/common/generate-filter.js#L99-L101
Added lines #L99 - L101 were not covered by tests
[warning] 106-106: src/auth-service/utils/common/generate-filter.js#L106
Added line #L106 was not covered by tests
[warning] 108-108: src/auth-service/utils/common/generate-filter.js#L108
Added line #L108 was not covered by tests
[warning] 111-111: src/auth-service/utils/common/generate-filter.js#L111
Added line #L111 was not covered by tests
[warning] 114-114: src/auth-service/utils/common/generate-filter.js#L114
Added line #L114 was not covered by tests
[warning] 116-117: src/auth-service/utils/common/generate-filter.js#L116-L117
Added lines #L116 - L117 were not covered by tests
| const headers = (req, res, next) => { | ||
| res.header("Access-Control-Allow-Origin", "*"); | ||
| res.header( | ||
| "Access-Control-Allow-Headers", | ||
| "Origin, X-Requested-With, Content-Type, Accept, Authorization" | ||
| ); | ||
| res.header("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, PATCH"); | ||
| next(); | ||
| }; | ||
| router.use(headers); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating CORS middleware.
You've defined an inline CORS headers middleware here, but there also appears to be a standalone headers.validators.js file with similar functionality. To maintain DRY principles and ensure consistent header application across routes, import and use the shared middleware instead.
-const headers = (req, res, next) => {
- res.header("Access-Control-Allow-Origin", "*");
- res.header(
- "Access-Control-Allow-Headers",
- "Origin, X-Requested-With, Content-Type, Accept, Authorization"
- );
- res.header("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, PATCH");
- next();
-};
-router.use(headers);
+const { headers } = require("@validators/common");
+router.use(headers);This assumes you've added the headers middleware to the exports in @validators/common/index.js.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const headers = (req, res, next) => { | |
| res.header("Access-Control-Allow-Origin", "*"); | |
| res.header( | |
| "Access-Control-Allow-Headers", | |
| "Origin, X-Requested-With, Content-Type, Accept, Authorization" | |
| ); | |
| res.header("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, PATCH"); | |
| next(); | |
| }; | |
| router.use(headers); | |
| const { headers } = require("@validators/common"); | |
| router.use(headers); |
| "/", | ||
| guestUserValidations.list, | ||
| setJWTAuth, | ||
| authJWT, | ||
| validate, | ||
| guestUserController.list | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Restrict guest user listing based on permissions.
The list endpoint allows fetching all guest users, but this should likely be restricted based on the authenticated user's permissions or tenant. Ensure the controller implements appropriate filtering based on the user's role and tenant.
🏁 Script executed:
#!/bin/bash
# Check how guest user listing is filtered
# Search for the implementation of list to see how it filters results
echo "Examining guest user listing implementation:"
rg -A 10 "list.*guest" --glob "src/auth-service/controllers/**"
echo "Checking if tenant filtering is applied:"
rg -A 3 "tenant" --glob "src/auth-service/controllers/guest-user.controller.js"Length of output: 3334
Action: Enhance Access Control for Guest User Listing
While tenant-based filtering is implemented in the guest user controller, there is no evidence of role-based permission checks. In addition to the tenant filter, please ensure that the guest user listing endpoint enforces restrictions based on the authenticated user's permissions or roles to prevent unauthorized access.
Recommendations:
- Confirm that role-based access control is applied either in the controller or via dedicated middleware.
- Consider adding explicit permission checks before invoking
guestUserUtil.list.
| router.post( | ||
| "/", | ||
| guestUserValidations.create, | ||
| validate, | ||
| guestUserController.create | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guest creation endpoint should validate tenant permissions.
The guest creation endpoint doesn't appear to have authentication middleware (setJWTAuth and authJWT), unlike the other endpoints. If this is intentional to allow unauthenticated guest creation, consider adding validation to prevent abuse, such as rate limiting or tenant verification.
🌐 Web query:
Are there any best practices for handling unauthenticated user registration with rate limiting?
💡 Result:
To handle unauthenticated user registration securely while preventing abuse, implement the following best practices combining rate limiting and security measures:
Rate Limiting Strategies
- IP-based limits: Restrict unauthenticated registration attempts per IP. GitLab recommends starting with 3,600 requests/hour per IP, adjustable based on traffic[2].
- Layered rules:
- Allow 4 requests/minute with a Managed Challenge (CAPTCHA) for initial blocks[3].
- Escalate to 10 requests/10 minutes for repeated violations[3].
- Block IPs exceeding 20 requests/hour for 24 hours[3].
- Response headers: Use headers like
Retry-AfterandRateLimit-Resetto inform clients when to retry[2][8].
Security Enhancements
- Avoid anonymous auth for attestation: Use AppCheck or similar tools instead of relying on anonymous tokens for app validation[1].
- Managed Challenges: Trigger CAPTCHA or proof-of-work challenges after rate limits to deter bots[3].
- Cleanup anonymous data: Automatically delete inactive anonymous accounts to reduce attack surfaces[1].
Registration Flow Design
- Simplify forms: Collect minimal data upfront and use multi-step flows for additional details[4].
- Offer social/enterprise logins: Reduce friction with OAuth or SAML integrations while shifting authentication burden to trusted providers[4].
- Passwordless options: Enable email/magic link signups to avoid password-related risks[4].
Monitoring and Adjustments
- Track headers: Monitor
RateLimit-ObservedandRateLimit-Remainingto detect abuse patterns[2]. - Temporary limit increases: For bulk operations, request short-term exceptions with clear justification (Okta’s process)[9].
For high-traffic systems, combine these with global rate limits (e.g., 100 requests/second across all endpoints as in Auth0[6]) and ensure backend rules block anonymous users from privileged actions[1].
Citations:
- 1: https://firebase.blog/posts/2023/07/best-practices-for-anonymous-authentication/
- 2: https://docs.gitlab.com/administration/settings/user_and_ip_rate_limits/
- 3: https://developers.cloudflare.com/waf/rate-limiting-rules/best-practices/
- 4: https://fusionauth.io/articles/identity-basics/registration-best-practices
- 5: https://buf.build/docs/bsr/rate-limits/
- 6: https://community.auth0.com/t/further-clarification-on-the-rate-limit-of-authentication-apis/35579
- 7: https://learn.microsoft.com/en-us/microsoft-365/solutions/best-practices-anonymous-sharing?view=o365-worldwide
- 8: https://docs.github.com/rest/using-the-rest-api/rate-limits-for-the-rest-api
- 9: https://developer.okta.com/docs/reference/rl-best-practices/
- 10: https://community.mendix.com/link/space/app-development/questions/10922
Action Required: Enhance Security on Guest Creation Endpoint
The guest creation endpoint (located at src/auth-service/routes/v2/guests.routes.js, lines 20–25) currently lacks tenant permission validation and does not include the authentication middlewares (setJWTAuth and authJWT) found in other endpoints. If allowing unauthenticated guest registration is intentional, please ensure that you mitigate potential abuse by:
- Implementing Rate Limiting: Apply IP-based limits (e.g., 3,600 requests/hour per IP), and consider layered strategies such as allowing a few requests per minute before escalating to CAPTCHA or temporary blocks. This helps manage request volume and discourages automated abuse.
- Adding Tenant Verification: Validate tenant details during guest creation to ensure that only authorized tenants can register.
- Consider Managed Challenges: Introducing mechanisms like CAPTCHA for repeated attempts can add an extra layer of security.
These adjustments align with industry best practices for handling unauthenticated user registration while protecting against abuse.
| const GuestUserModel = (tenant) => { | ||
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | ||
| const dbTenant = isEmpty(tenant) ? defaultTenant : tenant; | ||
| try { | ||
| let users = mongoose.model("guest_users"); | ||
| return users; | ||
| } catch (error) { | ||
| let users = getModelByTenant(dbTenant, "guest_user", GuestUserSchema); | ||
| return users; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify the model name to maintain consistency.
Currently, the code tries mongoose.model("guest_users"), then creates a new model named "guest_user". This mismatch can cause confusion or lead to missing model references. Consider using the same name for both.
Here's a potential fix:
- let users = getModelByTenant(dbTenant, "guest_user", GuestUserSchema);
+ let users = getModelByTenant(dbTenant, "guest_users", GuestUserSchema);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const GuestUserModel = (tenant) => { | |
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | |
| const dbTenant = isEmpty(tenant) ? defaultTenant : tenant; | |
| try { | |
| let users = mongoose.model("guest_users"); | |
| return users; | |
| } catch (error) { | |
| let users = getModelByTenant(dbTenant, "guest_user", GuestUserSchema); | |
| return users; | |
| } | |
| }; | |
| const GuestUserModel = (tenant) => { | |
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | |
| const dbTenant = isEmpty(tenant) ? defaultTenant : tenant; | |
| try { | |
| let users = mongoose.model("guest_users"); | |
| return users; | |
| } catch (error) { | |
| let users = getModelByTenant(dbTenant, "guest_users", GuestUserSchema); | |
| return users; | |
| } | |
| }; |
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/auth-service/models/GuestUser.js (3)
33-71: Consider improving success response logic in register methodIn the register method, I notice an inconsistency in the response structure. When a guest user is successfully created (lines 45-52), you return a success response with the created data, which is perfect. However, in the else block (lines 53-59), you still return
success: trueeven though the message indicates that the guest user was NOT successfully created.This inconsistency could confuse API consumers who might rely on the success flag to determine if the operation succeeded.
} else { return { - success: true, + success: false, data: createdGuestUser, message: "Operation successful but guest user NOT successfully created", status: httpStatus.OK, }; }
186-214: Rename custom findOne method to avoid shadowing Mongoose's native methodThe static method named
findOnepotentially shadows Mongoose's built-infindOnemethod. While your implementation works correctly, this naming overlap could confuse developers familiar with Mongoose's API and lead to unexpected behavior if someone attempts to use the native method directly.-async findOne({ filter = {}, next } = {}) { +async getOneGuestUser({ filter = {}, next } = {}) { try { const guestUser = await this.findOne(filter).exec(); if (!isEmpty(guestUser)) { return { success: true, data: guestUser, message: "successfully retrieved the guest user", status: httpStatus.OK, }; } else { return { success: false, message: "guest user not found", status: httpStatus.NOT_FOUND, }; } } catch (err) { logger.error(`🐛🐛 Internal Server Error -- ${err.message}`); next( new HttpError( "Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { message: err.message } ) ); } },
72-106: Consistent error handling and response formatting across methodsGreat job implementing consistent error handling and response formatting across all the static methods. Each method properly handles errors, logs them using the logger, and returns well-structured responses. This consistency makes the API more predictable and easier to work with.
However, consider extracting the error handling logic into a helper function since it's repeated in each method. This would make the code more DRY and easier to maintain.
Here's a suggestion for a helper function that could be defined above the schema:
const handleModelError = (err, next) => { logger.error(`🐛🐛 Internal Server Error -- ${err.message}`); next( new HttpError( "Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { message: err.message } ) ); };Then in each method, you could simplify the catch blocks:
} catch (err) { - logger.error(`🐛🐛 Internal Server Error -- ${err.message}`); - next( - new HttpError( - "Internal Server Error", - httpStatus.INTERNAL_SERVER_ERROR, - { message: err.message } - ) - ); + handleModelError(err, next); }Also applies to: 107-136, 137-165, 166-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/auth-service/models/GuestUser.js(1 hunks)src/auth-service/routes/v2/guests.routes.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
src/auth-service/models/GuestUser.js (3)
7-7: Kudos for including the 'isEmpty' importGreat job including the isEmpty utility that's used throughout the code. This demonstrates good attention to detail and ensures all dependencies are properly imported.
216-226: Unify the model name to maintain consistencyCurrently, the code tries
mongoose.model("guest_users"), then creates a new model named"guest_user". This mismatch can cause confusion or lead to missing model references. Consider using the same name for both.- let users = getModelByTenant(dbTenant, "guest_user", GuestUserSchema); + let users = getModelByTenant(dbTenant, "guest_users", GuestUserSchema);
13-32: Well-structured schema with appropriate indexesThe schema definition is well-structured with appropriate field types and validation. Good job adding indexes to
guest_idandlastActivefields - this will improve query performance for operations that filter or sort by these fields. The use of the timestamps option is also excellent for automatically tracking document creation and update times.src/auth-service/routes/v2/guests.routes.js (5)
6-9: Good use of common validator middlewareExcellent job using the common validator middleware. Importing the headers middleware from a common location ensures consistent CORS handling across routes.
11-16: Enhance Security on Guest Creation EndpointThe guest creation endpoint (located at
src/auth-service/routes/v2/guests.routes.js, lines 11–16) currently lacks tenant permission validation and does not include the authentication middlewares (setJWTAuthandauthJWT) found in other endpoints. If allowing unauthenticated guest registration is intentional, please ensure that you mitigate potential abuse by:
- Implementing Rate Limiting: Apply IP-based limits (e.g., 3,600 requests/hour per IP), and consider layered strategies such as allowing a few requests per minute before escalating to CAPTCHA or temporary blocks. This helps manage request volume and discourages automated abuse.
- Adding Tenant Verification: Validate tenant details during guest creation to ensure that only authorized tenants can register.
- Consider Managed Challenges: Introducing mechanisms like CAPTCHA for repeated attempts can add an extra layer of security.
These adjustments align with industry best practices for handling unauthenticated user registration while protecting against abuse.
24-31: Enhance Access Control for Guest User ListingWhile tenant-based filtering is implemented in the guest user controller, there is no evidence of role-based permission checks. In addition to the tenant filter, please ensure that the guest user listing endpoint enforces restrictions based on the authenticated user's permissions or roles to prevent unauthorized access.
Recommendations:
- Confirm that role-based access control is applied either in the controller or via dedicated middleware.
- Consider adding explicit permission checks before invoking
guestUserUtil.list.
17-23: Well-structured route handlers with consistent middlewareThe route handlers for converting, updating, deleting, and retrieving guest users are well-structured with appropriate HTTP methods, route paths, and middleware chains. Great job applying JWT authentication middleware consistently across these protected endpoints.
Also applies to: 32-55
1-5: Comprehensive import of required dependenciesGood job importing all the necessary dependencies for the router. The imports are well-organized and include the controller, validators, authentication middleware, and common validators.
Description
This pull request introduces a dedicated
GuestUsermodel and enhances guest user management.Key features include:
These changes allow for a more scalable and flexible system for handling unauthenticated users, particularly in scenarios where guest users are numerous and long-lived.
Changes Made
GuestUserModel:GuestUserSchemawith fields forguest_id,lastActiveandtenant.registerstatic function.liststatic function.modifystatic function.removestatic function.guestUser.util.js:createto create a guest user.listto retrieve multiple guest users.getOneto retrieve one guest user.updateto modify a guest user.deleteto remove a guest user.convertGuestto convert a guest user to a full user.guestUser.controller.js:createto handle the creation of a guest user.listto handle requests for multiple guest users.getOneto handle request for one guest user.updateto handle requests to update guest users.deleteto handle requests for deletion of guest users.convertGuestto handle the conversion of a guest user to a full user.guestUser.routes.js:POST /api/v2/users/gueststo create a guest user.GET /api/v2/users/gueststo list guest users.GET /api/v2/users/guests/:idto retrieve one guest user.PUT /api/v2/users/guests/:idto update a guest user.DELETE /api/v2/users/guests/:idto delete a guest user.POST /api/v2/users/guests/convertto convert a guest to a user.guestUser.validators.js:createvalidation for thePOST /api/v2/users/guests/route.listvalidation for theGET /api/v2/users/guests/route.getOnevalidation for theGET /api/v2/users/guests/:idroute.updatevalidation for thePUT /api/v2/users/guests/:idroute.deletevalidation for theDELETE /api/v2/users/guests/:idroute.convertGuestvalidation for thePOST /api/v2/users/guests/convertroute.paginationmiddleware.generate-filter.js:generateFilter.guest_usersto create filters for GuestUser queries.user.util.js:createGuestfunction.convertGuestfunction.user.controller.js:createGuestcontroller.convertGuestcontroller.user.routes.js:user.validators.js:Testing
Affected Services
Endpoints Ready for Testing
New endpoints ready for testing:
POST /api/v2/users/guests
GET /api/v2/users/guests
GET /api/v2/users/guests/:id
PUT /api/v2/users/guests/:id
DELETE /api/v2/users/guests/:id
POST /api/v2/users/guests/convert
API Documentation Updated?
Additional Notes
Summary by CodeRabbit
Summary by CodeRabbit